refactor: Add slackcontext package to DRY context management #7

mwbrooks · 2025-04-02T19:49:24Z

Summary

This pull request introduces the package slackcontext to allow context to be managed in one place.

Usage:

Any package can get context/state values using slackcontext.<Function>
Any package can set context/state values using slackcontext.<SetFunction>
Every command is executed with the context (.ExecuteContext(ctx)) so cmd.Context() returns the correct context

Thoughts:

We don't want to over-use ctx
There are split opinions in the Golang community on what can go into ctx
Common theme is that it should contain the state for a specific "request"
- In HTTP world, that would be a request to a HTTP route
- In CLI world, it's less clear, but likely the execution of the command
Since ctx is immutable, any function that modifies it must return the new ctx
For this reason, it's best to reduce the amount that ctx is updated
Right now, we are only updating the ctx before rootCmd.ExecuteContext(ctx)

Rule of thumb:

For now, I think it'll be best to assume ctx doesn't change after rootCmd.ExecuteContext(ctx) is called
The ctx should not contain required function args, such as fs or os so we'll need to think about where these should live long-term

Reviewers

Sorry about the massive PR. Most of the PR is updating tests to use slackcontext.MockContext(ctx) instead of context.Background(). This is because values such as Version are required by the API Client.

There should be no change to functionality.

Requirements

I've read and understood the Contributing Guidelines and have done my best effort to follow them.
I've read and agree to the Code of Conduct.

codecov · 2025-04-02T19:51:00Z

Codecov Report

Attention: Patch coverage is 61.80556% with 55 lines in your changes missing coverage. Please review.

Project coverage is 62.94%. Comparing base (f2f33c4) to head (aae77de).
Report is 15 commits behind head on main.

Files with missing lines	Patch %	Lines
main.go	0.00%	15 Missing ⚠️
internal/iostreams/logfile.go	0.00%	10 Missing ⚠️
internal/api/client.go	25.00%	6 Missing and 3 partials ⚠️
internal/slackcontext/slackcontext.go	90.47%	6 Missing and 2 partials ⚠️
cmd/root.go	55.55%	4 Missing ⚠️
internal/api/icon.go	25.00%	2 Missing and 1 partial ⚠️
internal/api/s3_upload.go	25.00%	2 Missing and 1 partial ⚠️
internal/tracking/tracking.go	25.00%	2 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main       #7      +/-   ##
==========================================
+ Coverage   62.78%   62.94%   +0.16%     
==========================================
  Files         210      210              
  Lines       22053    22127      +74     
==========================================
+ Hits        13846    13928      +82     
+ Misses       7128     7115      -13     
- Partials     1079     1084       +5

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

mwbrooks · 2025-04-02T20:25:42Z

cmd/env/list.go

 		},
 		RunE: func(cmd *cobra.Command, args []string) error {
-			return runEnvListCommandFunc(clients)
+			return runEnvListCommandFunc(clients, cmd)


While runEnvListCommandFunc(ctx, clients, cmd) would be a more appropriate format, the other env commands only pass cmd and fetch the context with cmd.Context().

This change is leaning on consistency and a smaller change footprint, since the PR is already quite large.

mwbrooks · 2025-04-02T20:25:57Z

cmd/env/list.go

+	cmd *cobra.Command,
 ) error {
-	ctx := context.Background()
+	ctx := cmd.Context()


Fixed getting the real context, so that the env list command can access the Slack CLI Version. Currently, all API requests for the env list command don't include the Version in the user-agent.

mwbrooks · 2025-04-02T20:26:43Z

cmd/root.go

+		// TODO(slackcontext) Consolidate storing SystemID to slackcontext
 		clients.Config.SystemID = systemID


This would be a larger refactor that can wait until we 🪓 axe storing state in clients.Config

mwbrooks · 2025-04-02T20:27:19Z

cmd/root.go

+		// TODO(slackcontext) Consolidate storing ProjectID to slackcontext
 		clients.Config.ProjectID = projectID


Likewise, this can wait until we 🪓 axe storing state in clients.Config

mwbrooks · 2025-04-02T20:28:35Z

cmd/root.go

-	// TODO: ensure context is only used with setters and not with getters. After investigating, report:
-	// - internal/contextutil/contextutil.go has a `VersionFromContext` getter method which is used in various API methods when setting the user agent on HTTP requests, and when sending data up to logstash
-	// - internal/config/context.go has a variety of set and get methods related to app, token, team/enterprise/session/user/trace IDs, domains


These files have been refactored to slackcontext and the comment is no longer relevant.

mwbrooks · 2025-04-02T20:57:46Z

internal/api/client.go

+
+	cliVersion, err := slackcontext.Version(ctx)
+	if err != nil {
+		return nil, err
+	}


These errors are bubbled up to the original caller. Previously, when version didn't exist, the API calls would be made with a "" empty string.

mwbrooks · 2025-04-02T21:06:18Z

internal/config/context.go

 type contextKey string

-const CONTEXT_ENV contextKey = "env"
 const CONTEXT_TOKEN contextKey = "token"


The remaining functions in this file don't belong in the slackcontext because they deal with run-time state - selected auth, team, app, etc. We may want to refactor this into a standardized selected app struct that can be passed around.

mwbrooks · 2025-04-02T21:07:50Z

internal/slackcontext/slackcontext_test.go

+	"github.com/stretchr/testify/require"
+)
+
+func Test_SlackContext_OpenTracingSpan(t *testing.T) {


Slightly disappointed that removing the untested contextutil.go and some of context.go didn't result in a higher test coverage increase. Oh well 🤷🏻

mwbrooks · 2025-04-02T21:09:59Z

main.go

+	// Create the parent context for the CLI execution
 	var ctx = context.Background()
+


This should be the one and only time we call context.Background(). In a future PR, I'll scrub the code base for all references to it, because I'm sure there are still some.

mwbrooks · 2025-04-02T21:10:32Z

main.go

+		ctx = slackcontext.SetSessionID(ctx, uuid.New().String())
+


In a follow-up PR, I'd like to clean this up so that we can use the existing SessionID rather than generating a new one when there is an unrecoverable error.

zimeg

@mwbrooks What an incredible improvement to the codebase this is 🙏 ✨

All of these changes make a lot of sense to me and I like how you're thinking about the setting of context values in "root" a lot! This, and the error handling within, makes these callsites much more reliable IMO.

A comment below asks a question about possible bugs discovered, but I'm excited for these changes to merge too. All things are compiling and running well for me! 🚢 💨

zimeg · 2025-04-08T20:39:23Z

cmd/root_test.go

+	ctx := slackcontext.MockContext(t.Context())
+


@mwbrooks This is wonderful with the setup in root!

slack-cli/internal/slackcontext/slackcontext_mock.go

Lines 23 to 34 in aae77de

const (

mockCLIVersion = "v1.2.3"

)

// MockContext sets values in the context that are guaranteed to exist before

// the Cobra root command is executed.

func MockContext(ctx context.Context) context.Context {

ctx = SetOpenTracingTraceID(ctx, uuid.New().String())

ctx = SetSessionID(ctx, uuid.New().String())

ctx = SetVersion(ctx, mockCLIVersion)

return ctx

}

zimeg · 2025-04-08T20:41:53Z

cmd/root.go

 			}()
 		case <-ctx.Done():
 			// No interrupt signal sent, command executed to completion
+			// FIXME - `.Done()` channel is triggered by `cancel()` and not a successfully completion


I'm super curious about this 👀

It's not super clear what the bug is but I'm not doubting that this might be causing problems... Were you noticing something strange during command completion?

zimeg · 2025-04-08T20:57:15Z

cmd/root.go

-		if span := opentracing.SpanFromContext(ctx); span != nil {
-			span.SetTag("project_id", clients.Config.ProjectID)
-			ctx = opentracing.ContextWithSpan(ctx, span)
-		}
-
+		ctx = slackcontext.SetProjectID(ctx, projectID)
+		rootCmd.SetContext(ctx)


I don't think we need to do this, because the rootCmd.ExecuteContext(ctx) will execute the command with our context. However, if someone called rootCmd.Context() before executing the Cobra command, they'll at least get the correct context.

zimeg · 2025-04-08T21:01:44Z

cmd/root_test.go

-	err := cmd.Execute()
+	err := cmd.ExecuteContext(ctx)


TIL! Is this the same as Execute, but sets the ctx on the command?

Edit: Just saw the note in the PR description - this is neat! 🙏 📚

You got it!

cmd.Execute() will use a context.Background() (empty context)

cmd.ExecuteContext(ctx) will use ctx

Whenever someone called cmd.Context() they'll get whatever context has been set. We want to ensure that they always get our context.

mwbrooks · 2025-04-08T23:31:35Z

@zimeg Thanks so much for lending your time to review this PR! 🙇🏻 Tedious and long, but I hope it positions us to manage our context in context of the context 😳

refactor: Add slackcontext package to DRY context management

4add4cc

mwbrooks added code health M-T: Test improvements and anything that improves code health semver:patch Use on pull requests to describe the release version increment labels Apr 2, 2025

mwbrooks added this to the Next Release milestone Apr 2, 2025

mwbrooks self-assigned this Apr 2, 2025

refactor: Revert changes to unrelated error codes

6c8907a

mwbrooks commented Apr 2, 2025

View reviewed changes

Merge branch 'main' into mbrooks-slackcontext

aae77de

mwbrooks marked this pull request as ready for review April 3, 2025 16:19

mwbrooks requested a review from a team as a code owner April 3, 2025 16:19

zimeg modified the milestones: v3.0.4, Next Release Apr 4, 2025

mwbrooks mentioned this pull request Apr 7, 2025

test: mock context in TableTestCommand #20

Merged

2 tasks

zimeg approved these changes Apr 8, 2025

View reviewed changes

Merge branch 'main' into mbrooks-slackcontext

a16ba4c

mwbrooks merged commit b82e4c3 into main Apr 8, 2025
6 checks passed

mwbrooks deleted the mbrooks-slackcontext branch April 8, 2025 23:31

		// TODO(slackcontext) Consolidate storing SystemID to slackcontext
		clients.Config.SystemID = systemID

		// TODO(slackcontext) Consolidate storing ProjectID to slackcontext
		clients.Config.ProjectID = projectID

		// Create the parent context for the CLI execution
		var ctx = context.Background()

	const (
	mockCLIVersion = "v1.2.3"
	)

	// MockContext sets values in the context that are guaranteed to exist before
	// the Cobra root command is executed.
	func MockContext(ctx context.Context) context.Context {
	ctx = SetOpenTracingTraceID(ctx, uuid.New().String())
	ctx = SetSessionID(ctx, uuid.New().String())
	ctx = SetVersion(ctx, mockCLIVersion)
	return ctx
	}

refactor: Add slackcontext package to DRY context management #7

refactor: Add slackcontext package to DRY context management #7

Uh oh!

Conversation

mwbrooks commented Apr 2, 2025

Summary

Reviewers

Requirements

Uh oh!

codecov bot commented Apr 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zimeg left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mwbrooks commented Apr 8, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

codecov bot commented Apr 2, 2025 •

edited

Loading